Repository navigation
Encode path option get and update - #243
Conversation
|
|
|
@linda-sap Welcome to the contributors. This is a nice pull request. Could you just explain why is the encoding optional? Isn't just a bug to not always encode it? |
|
@jfilak Thanks :) I currently can't think of a case where it shouldn't be encoded, rather just to not crash any existing use, where it might be expected unencoded. Would you opt for making it always encode or maybe to make the encoding the default? |
|
@linda-sap yes, please swap the logic - make encoding default and offer the possibility to not encode for possible workarounds. |
Agree, it is a bugfix, but a for five years old part that may be a possible unforeseen breaking change for somebody. At least the workaround option for run of behavior that existed so far. I would stress out more the fact that this was reproduced on SAP Gateway for particular service, and so far no issue for PUT/Batch payload for other odata services was created by any other library user. Perhaps they did not run into a problem with batch, but perhaps it is a SAP Gateway specific problem. Removal of this configuration option may be IMHO a good subtask for 2.0 version - if encoding of everything in batch payload is as correct as the encoding in the url query params. |
|
@linda-sap please add a changelog entry under the Unreleased as well, so it is not forgotten. |
|
@linda-sap The linter job did not run, but it is important for this PR as well as a gate-keeper. I have fixed it in PR #239. Please update your branch as well, so we do not see this problem. |
…ncode-path-option-get-and-update
|
@phanak-sap do I get you right, that you also prefer the option to make the encoding the default and leave the option to witch it off? I just want to be sure as also a good amount of tests need a change for this. |
Yes. I would probably start more defensively exactly like you did :), but the @jfilak suggestion makes for sense for long future. Bugfix, not a new feature mentality. Encoding by default, with new parameter option to run old behavior for possible workarounds. |
|
@jfilak @phanak-sap I changed it so the encoding is done by default and added the encoding to delete and getEntitySet request. Also I adopted the tests. However I'm not sure what the best way is to include the encoding or not in the get_value() function. Whether the general ODataHttpRequest class should handle it. |
|
Hi @linda-sap, you have failing one test in current PR - FAILED tests/test_service_v2.py::test_entity_get_value_1on1_with_proxy - requests.exceptions.ConnectionError: Connection refused by Responses - the call doesn't match any registered mock. Please fix the linter build as well to be passing. You can run it locally as well, check "pylint" and "flake8": in the contributing guide Two things from previous build were fixed, these remains. I am OK with pyodata/v2/service.py:578: [R0902(too-many-instance-attributes), EntityModifyRequest] Too many instance attributes (8/7) |
|
@phanak-sap thanks a lot for your feedback! |
I do not understand "those are not the requests to be sent." correctly perhaps. Are we talking about method that basically add Maybe this part of documentation will tie better the odata documentation and this library: |
Okay, I just wanted to make sure there is no need to also encode the url in these methods. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #243 +/- ##
==========================================
- Coverage 92.34% 92.31% -0.04%
==========================================
Files 6 6
Lines 2849 2874 +25
==========================================
+ Hits 2631 2653 +22
- Misses 218 221 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Well, So I am under impression that everything is covered and this is just "communication noise" :). I will wait some time for possible reaction, but I will merge this today or tomorrow. |
Correct, the get_path() is encoded and the others not. |
|
Thank you for all the work. |
The problem and fix:
urllib.parse.quote() defaults to safe='/', so a slash inside a key value
(e.g. SoftwareComponents('/BAZ/FOO')) was left unencoded in the request
path. The entity-set name and key string were quoted as one string,
making it impossible to encode key-value slashes while preserving the
structural slashes that separate navigation segments.
Add encode_entity_path(), a parenthesis-depth-aware encoder: text outside
the key predicate is quoted with safe='/' (structural separators kept),
text inside (...) with safe='' (every reserved char, including /, encoded).
Output is byte-identical to the old quote() for keys without slashes, so
no existing path changes. Applied to EntityGetRequest, EntityDeleteRequest,
EntityModifyRequest and GetEntitySetRequest; batch inherits it via
get_path(). encode_path=False still bypasses encoding unchanged.
Additional notes:
This is outbound-only by design and has no from_literal-style counterpart:
encode_entity_path runs solely in get_path() to build request URLs. pyodata
never decodes a key back out of a path (the server does the percent-decoding;
the __next link is reused verbatim), so there is no reverse direction to
keep symmetric.
The encode_path=False workaround from PR #243 is intentionally left in
place for backward compatibility: callers who pre-encode keys and disable
encoding must keep working unchanged, and the flag also serves as a
raw-path override for non-conformant servers. Removing it is deferred to
a future major release.
Fixes #282
Sometimes services expect the path to be percent encoded. This can especially be important
when special variable types are key fields like Date type, where a ':' will appear in the path.
In this case you can use an optional parameter to make the request encode the path.
Per default the variable encode_path is set to False.